[WIP] Add Kimi Agentic with LMCache NIXL#2171
Conversation
YukioZzz
requested review from
1am9trash,
billishyahao,
chunfangamd,
seungrokj and
yctseng0211
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
YukioZzz
changed the title
YukioZzz
force-pushed
the
yichaozhu/kimi-dep-lmcache-p2p
branch
from
8078ef7 to
a359f72
Compare
YukioZzz
force-pushed
the
yichaozhu/kimi-dep-lmcache-p2p
branch
from
7636bfa to
1545b53
Compare
# Conflicts: # benchmarks/multi_node/amd_utils/job.slurm # benchmarks/multi_node/amd_utils/models_vllm.yaml # perf-changelog.yaml
|
Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase For PR verification, add the PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs 感谢你的贡献！请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md，然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人，你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板，包括保留英文语句 如需进行 PR 验证，请为此 PR 添加 PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动（flake），重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档 |
|
Claude finished @cquil11's task in 7m 49s —— View job Re-review: Kimi K2.7 DEP LMCache NIXLThe changed-file set has shifted substantially since the prior review (the workflow-heredoc file and the single-node script are no longer part of this PR), so I re-reviewed the full current diff against Todo
Previously-flagged issues — all resolved ✅
New review of current diffI checked the higher-risk changes and did not find blocking bugs:
Minor (non-blocking)
LGTM — no blocking issues found. Note the PR title still says |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29310181345 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29310365832 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29310896474 |
|
|
||
| # ============================================================================= | ||
| # KV connector selection | ||
| # ============================================================================= | ||
| _MORIIO_EXTRA="\"kv_connector_extra_config\": {\"proxy_ip\": \"${NODE0_ADDR}\", \"proxy_ping_port\": \"${PROXY_PING_PORT}\", \"http_port\": \"${SERVER_PORT}\"}" |
There was a problem hiding this comment.
🔴 The prefill/decode --kv-transfer-config for MoRIIOConnector previously hardcoded "read_mode": true inside kv_connector_extra_config; the new MORIIO_PREFILL_CONN/MORIIO_DECODE_CONN templates in server_vllm.sh (used as the default KVT_PREFILL/KVT_DECODE for every existing moriio recipe, not just the new DEP recipe) drop that key entirely and rely instead on job.slurm's -e VLLM_MORIIO_CONNECTOR_READ_MODE=${VLLM_MORIIO_CONNECTOR_READ_MODE:-1}. This is a pre-existing issue in the sense that it affects already-productionized recipes (kimik2.5-fp4-mi355x-vllm-disagg, minimaxm3-fp4/fp8-mi355x-vllm-disagg) rather than the new DEP recipe this PR adds, but the regression itself is introduced by this PR's refactor of the shared server_vllm.sh KV-connector logic, and perf-changelog.yaml (PR #1585) explicitly documents that the env var was deliberately abandoned in favor of the extra_config key for the v0.24.0 image those recipes still use.
Extended reasoning...
What changed and why it matters
Before this PR, server_vllm.sh built the vLLM --kv-transfer-config JSON inline for each of the three MoRIIOConnector launch sites (rank-0 prefill, additional prefill nodes, decode nodes), and that JSON hardcoded "read_mode": true inside kv_connector_extra_config. This PR refactors that into reusable templates — MORIIO_PREFILL_CONN / MORIIO_DECODE_CONN (server_vllm.sh:264-268) — which are used as KVT_PREFILL/KVT_DECODE by default for the moriio|"" case arm. Critically, that default arm is a no-op — it does not fall back to a different template that preserves read_mode. The new templates only carry proxy_ip, proxy_ping_port, and http_port; the read_mode key is gone.
To compensate, job.slurm now exports -e VLLM_MORIIO_CONNECTOR_READ_MODE=${VLLM_MORIIO_CONNECTOR_READ_MODE:-1} into the container (job.slurm:473). The intent is clearly to preserve READ-mode behavior via an environment variable instead of the JSON config key.
Why this is risky rather than just a refactor
perf-changelog.yaml (entry for PR #1585, config-key kimik2.5-fp4-mi355x-vllm-disagg) documents that this exact env var was deliberately deprecated for the vllm/vllm-openai-rocm:v0.24.0 image:
Bump image to vllm/vllm-openai-rocm:v0.24.0 (MoRI-IO fixes upstreamed in vllm#40344) ... set read_mode via kv_connector_extra_config instead of VLLM_MORIIO_CONNECTOR_READ_MODE env var.
kimik2.5-fp4-mi355x-vllm-disagg in configs/amd-master.yaml still pins vllm/vllm-openai-rocm:v0.24.0 today, and minimaxm3-fp4-mi355x-vllm-disagg / minimaxm3-fp8-mi355x-vllm-disagg route through the same shared server_vllm.sh moriio templates. None of these recipes set PREFILL_KV_CONNECTOR/DECODE_KV_CONNECTOR to anything other than the moriio default, so they all pick up MORIIO_PREFILL_CONN/MORIIO_DECODE_CONN and lose the extra_config read_mode key.
Concrete walk-through
- Before:
kimik2.5-fp4-mi355x-vllm-disaggprefill launch included--kv-transfer-config '{"kv_connector": "MoRIIOConnector", "kv_role": "kv_producer", "kv_connector_extra_config": {..., "read_mode": true}}'. - After this PR: the same recipe (unchanged additional-settings, still on v0.24.0) now gets
KVT_PREFILL=$MORIIO_PREFILL_CONN, whosekv_connector_extra_confighas noread_modekey at all. - job.slurm sets
VLLM_MORIIO_CONNECTOR_READ_MODE=1in the container env, but per the [Fix] Remove MoRI-IO patches from vLLM Disagg benchmarks #1585 changelog, that env var was specifically superseded because v0.24.0's connector readsread_modefromkv_connector_extra_config, not the environment. - If the connector does not consult the env var on this image (which is exactly what "instead of" implies),
read_modesilently reverts to the connector's internal default, which — absent an explicittrue— is WRITE mode. - WRITE mode is precisely the failure path that the
setup_deps.shMoRIIO patches (save_kv_layerbusy-spin/deadlock fix, notify-parser fix) exist to work around; this PR carries no test or assertion that would catch the regression, and none of the three affected recipes are part of this PR's own new/tested surface (only the new lmcache-nixl DEP recipe is).
Fix
Either keep read_mode in the kv_connector_extra_config templates for the moriio case (so existing recipes are unaffected regardless of which image/connector version they run), or confirm the specific vLLM/connector version each affected recipe uses actually honors VLLM_MORIIO_CONNECTOR_READ_MODE before relying on it exclusively. Given kimik2.5-fp4-mi355x-vllm-disagg is still pinned to v0.24.0, the safer fix is the former.
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29311324452 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29312221875 |
|
see unofficial run visualizer at https://inferencex.semianalysis.com/inference?unofficialRun=29312239774 |
2 participants
Adds a MI355X Kimi-K2.7-Code-MXFP4 DEP vLLM disagg CI point using MultiConnector[NixlConnector + LMCacheMPConnector] with LMCache 1.2T and P2P disabled for the 1P1D bring-up path.\n\nAlso includes the LMCacheMP new-request block tracking fix and static vLLM router endpoints for lmcache-nixl.